-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Execution context] Add nested context support #107523
[Execution context] Add nested context support #107523
Conversation
ExecutionContextContaier is not compatible with SerializableState, so I had to fall back to passing context as POJO. With this change, using a service looks like overhead.
Pinging @elastic/kibana-core (Team:Core) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app services code LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dashboard changes look fine to me!
Wanted to get to this today but ran out of time, will do tomorrow - Aug 6 |
@@ -34,7 +34,7 @@ export interface IExecutionContextContainer { | |||
} | |||
|
|||
export class ExecutionContextContainer implements IExecutionContextContainer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this class would make more sense to be named as ExecutionContextSerializer
now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will have different abstractions on the client and server then. I'd keep it as is. Considering that it's an internal API, we can always rename it later.
src/core/public/execution_context/execution_context_container.ts
Outdated
Show resolved
Hide resolved
src/core/public/execution_context/execution_context_service.mock.ts
Outdated
Show resolved
Hide resolved
description: 'new-description', | ||
}; | ||
router.get({ path: '/execution-context', validate: false }, async (context, req, res) => { | ||
const { headers } = await executionContext.withContext(newContext, () => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we provide executionContext.withContext
on RequestHandlerContext
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd wait until the first use case.
I don't think lots of endpoints will use this API manually to create a domain-specific context. For most of the cases, instrumentation performed by Core will be enough.
name: 'name-b', | ||
id: 'id-b', | ||
description: 'description-b', | ||
parent: undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious if this is the behavior we actually want instead of subsequent calls using the previous context as the parent. If pluginA
calls a sync API on pluginB
and they both set context, wouldn't we want to track that relationship?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If pluginA calls a sync API on pluginB and they both set context, wouldn't we want to track that relationship?
set
is Core-specific API which won't be provided to plugins. withContext
which is exposed to plugins, supports context stacking
it('supports nested contexts', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KibanaApp owned code LGTM, tested adding TSVB + Lens visualization to Dashboard, since embeddable code was modified, works as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsAPI count
API count missing comments
History
To update your PR or re-run it, just comment with: |
* Add nested context support * remove execution context service on the client side ExecutionContextContaier is not compatible with SerializableState, so I had to fall back to passing context as POJO. With this change, using a service looks like overhead. * update docs * fix test * address comments from Josh * put export back * update docs * remove outdated export * use input.title for unsaved vis Co-authored-by: Kibana Machine <[email protected]>
* Add nested context support * remove execution context service on the client side ExecutionContextContaier is not compatible with SerializableState, so I had to fall back to passing context as POJO. With this change, using a service looks like overhead. * update docs * fix test * address comments from Josh * put export back * update docs * remove outdated export * use input.title for unsaved vis Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Summary
Part of #102626
Adds support for declaring parent-child relationships between execution contexts. With this change, Kibana might log and pass to Elasticsearch information about the origin of every spawned execution context.
kibana logs
x-opaque-id header
Client-side API
Due to lack of native JS API, we have to pass a parent context during instantiation of a child context:
Note that
EmbeddableInput
is expected to be compatible withSerializableState
interface, so I had to removeIExecutionContextContainer
usage in favor ofKibanaExecutionContext
which is POJO. With this change, usingexecution_context
service on the client-side looks like overhead, so I removed it completely.Server-side API
On the server-side, plugins may register
execution context
by wrapping any function inwithContext
. Provided context object is carried over all async operations spawned by the passed function. Sequentual callswithContext
will create a stack of context.How to test
Sample eCommerce orders
.Checklist
Delete any items that are not applicable to this PR.
For maintainers